From cb369f71429b6122949668ce6fbaf9540b90ceca Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 21 Aug 2025 11:57:03 +0200 Subject: [PATCH] Deduplicate repo+sysroot syncfs logic This is a followup to https://github.com/ostreedev/ostree/pull/3504/commits/6e5a27a29d33d50a2a4380c406405435d919b6b4 which I believe is correct as is. However, we already have a file descriptor open for the ostree repo, which *must* be on the same filesystem as `/sysroot/ostree` (the deployment code forces hardlinking today). It's hence cleaner to reuse that extant fd instead of opening a new one - we know we did writes to that fd. But going farther here, there already is logic to use syncfs for the repo when downloading objects (in a common case we actually syncfs twice). Since these are really the same operation, unify them: - Add journaling to the repo one syncfs case - Change the sysroot case to just call it - Since we log consistently to the journal for all syncfs/fsfreeze operations now, drop the SyncStats bits which was a way to add info about that to a later journal message Additionally, let's add an extra check when we're opening the repo that it's on the same device just on general principle. Signed-off-by: Colin Walters --- src/libostree/ostree-repo-commit.c | 10 +--- src/libostree/ostree-repo-private.h | 1 + src/libostree/ostree-repo.c | 26 ++++++++ src/libostree/ostree-sysroot-deploy.c | 66 ++++++++------------- src/libostree/ostree-sysroot.c | 20 +++++++ tests/kolainst/destructive/staged-deploy.sh | 7 +++ 6 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 4bf845c4..36240fae 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2227,14 +2227,8 @@ ostree_repo_commit_transaction (OstreeRepo *self, OstreeRepoTransactionStats *ou if ((self->test_error_flags & OSTREE_REPO_TEST_ERROR_PRE_COMMIT) > 0) return glnx_throw (error, "OSTREE_REPO_TEST_ERROR_PRE_COMMIT specified"); - /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know - * about `syncfs`...we should delete this later. - */ - if (!self->disable_fsync && g_getenv ("OSTREE_SUPPRESS_SYNCFS") == NULL) - { - if (syncfs (self->tmp_dir_fd) < 0) - return glnx_throw_errno_prefix (error, "syncfs(repo/tmp)"); - } + if (!_ostree_repo_syncfs (self, error)) + return FALSE; if (!rename_pending_loose_objects (self, cancellable, error)) return FALSE; diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 3ea1ff65..77ea2a6b 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -394,6 +394,7 @@ gboolean _ostree_repo_load_file_bare (OstreeRepo *self, const char *checksum, in GError **error); gboolean _ostree_repo_update_mtime (OstreeRepo *self, GError **error); +gboolean _ostree_repo_syncfs (OstreeRepo *self, GError **error); gboolean _ostree_repo_add_remote (OstreeRepo *self, OstreeRemote *remote); gboolean _ostree_repo_remove_remote (OstreeRepo *self, OstreeRemote *remote); diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 770846a8..3f2f0419 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1521,6 +1521,32 @@ _ostree_repo_update_mtime (OstreeRepo *self, GError **error) return TRUE; } +gboolean +_ostree_repo_syncfs (OstreeRepo *self, GError **error) +{ + + if (self->disable_fsync) + return TRUE; + /* FIXME: Added OSTREE_SUPPRESS_SYNCFS since valgrind in el7 doesn't know + * about `syncfs`...we should delete this later. + */ + if (g_getenv ("OSTREE_SUPPRESS_SYNCFS") != NULL) + return TRUE; + + gboolean is_system = ostree_repo_is_system (self); + if (is_system) + ot_journal_print (LOG_INFO, "Starting syncfs for system repo"); + guint64 start_msec = g_get_monotonic_time () / 1000; + int repo_dfd = ostree_repo_get_dfd (self); + if (syncfs (repo_dfd) != 0) + return glnx_throw_errno_prefix (error, "syncfs(repo)"); + guint64 end_msec = g_get_monotonic_time () / 1000; + if (is_system) + ot_journal_print (LOG_INFO, "Completed syncfs() for system repo in %" G_GUINT64_FORMAT " ms", + end_msec - start_msec); + return TRUE; +} + /** * ostree_repo_get_config: * @self: diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c index 4abd7001..3bfc8d9f 100644 --- a/src/libostree/ostree-sysroot-deploy.c +++ b/src/libostree/ostree-sysroot-deploy.c @@ -1628,44 +1628,31 @@ fsfreeze_thaw_cycle (OstreeSysroot *self, int rootfs_dfd, GCancellable *cancella return TRUE; } -typedef struct -{ - guint64 root_syncfs_msec; - guint64 boot_syncfs_msec; -} SyncStats; - /* sync /ostree and /boot which may be separate mount points. */ static gboolean -full_system_sync (OstreeSysroot *self, SyncStats *out_stats, GCancellable *cancellable, - GError **error) +full_system_sync (OstreeSysroot *self, GCancellable *cancellable, GError **error) { GLNX_AUTO_PREFIX_ERROR ("Full sync", error); - ot_journal_print (LOG_INFO, "Starting syncfs() for /ostree"); - guint64 start_msec = g_get_monotonic_time () / 1000; - glnx_autofd int ostree_dfd = -1; - if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_dfd, error)) - return glnx_throw_errno_prefix (error, "glnx_opendirat(/ostree)"); - if (syncfs (ostree_dfd) != 0) - return glnx_throw_errno_prefix (error, "syncfs(/ostree)"); - guint64 end_msec = g_get_monotonic_time () / 1000; - ot_journal_print (LOG_INFO, "Completed syncfs() for /ostree in %" G_GUINT64_FORMAT " ms", - end_msec - start_msec); - - out_stats->root_syncfs_msec = (end_msec - start_msec); if (!_ostree_sysroot_ensure_boot_fd (self, error)) return FALSE; - g_assert_cmpint (self->boot_fd, !=, -1); + + // Must have been initialized + g_assert (self->repo); + // Because we require that /ostree is on the same fs as /ostree/repo, reuse + // the logic to syncfs the repo fd. + if (!_ostree_repo_syncfs (self->repo, error)) + return FALSE; + ot_journal_print (LOG_INFO, "Starting freeze/thaw cycle for boot"); - start_msec = g_get_monotonic_time () / 1000; + guint64 start_msec = g_get_monotonic_time () / 1000; if (!fsfreeze_thaw_cycle (self, self->boot_fd, cancellable, error)) return FALSE; - end_msec = g_get_monotonic_time () / 1000; + guint64 end_msec = g_get_monotonic_time () / 1000; ot_journal_print (LOG_INFO, "Completed freeze/thaw cycle for boot in %" G_GUINT64_FORMAT " ms", end_msec - start_msec); - out_stats->boot_syncfs_msec = (end_msec - start_msec); return TRUE; } @@ -2410,8 +2397,7 @@ ostree_sysroot_write_deployments (OstreeSysroot *self, GPtrArray *new_deployment static gboolean write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments, OstreeSysrootWriteDeploymentsOpts *opts, OstreeBootloader *bootloader, - SyncStats *out_syncstats, char **out_subbootdir, - GCancellable *cancellable, GError **error) + char **out_subbootdir, GCancellable *cancellable, GError **error) { const int new_bootversion = self->bootversion ? 0 : 1; @@ -2464,7 +2450,7 @@ write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments, if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, cancellable, error)) return FALSE; - if (!full_system_sync (self, out_syncstats, cancellable, error)) + if (!full_system_sync (self, cancellable, error)) return FALSE; if (!swap_bootloader (self, bootloader, self->bootversion, new_bootversion, cancellable, error)) @@ -2994,9 +2980,6 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n return glnx_throw (error, "Attempting to remove booted deployment"); gboolean bootloader_is_atomic = FALSE; - SyncStats syncstats = { - 0, - }; g_autoptr (OstreeBootloader) bootloader = NULL; g_autofree char *new_subbootdir = NULL; if (!requires_new_bootversion) @@ -3004,7 +2987,7 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n if (!create_new_bootlinks (self, self->bootversion, new_deployments, cancellable, error)) return FALSE; - if (!full_system_sync (self, &syncstats, cancellable, error)) + if (!full_system_sync (self, cancellable, error)) return FALSE; if (!swap_bootlinks (self, self->bootversion, new_deployments, &new_subbootdir, cancellable, @@ -3020,8 +3003,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader); - if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, &syncstats, - &new_subbootdir, cancellable, error)) + if (!write_deployments_bootswap (self, new_deployments, opts, bootloader, &new_subbootdir, + cancellable, error)) return FALSE; } @@ -3032,15 +3015,14 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self, GPtrArray *n requires_new_bootversion ? "yes" : "no", new_subbootdir, new_deployments->len - self->deployments->len); const gchar *bootloader_config = ostree_repo_get_bootloader (ostree_sysroot_repo (self)); - ot_journal_send ( - "MESSAGE_ID=" SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL (OSTREE_DEPLOYMENT_COMPLETE_ID), - "MESSAGE=%s", msg, "OSTREE_BOOTLOADER=%s", - bootloader ? _ostree_bootloader_get_name (bootloader) : "none", - "OSTREE_BOOTLOADER_CONFIG=%s", bootloader_config, "OSTREE_BOOTLOADER_ATOMIC=%s", - bootloader_is_atomic ? "yes" : "no", "OSTREE_DID_BOOTSWAP=%s", - requires_new_bootversion ? "yes" : "no", "OSTREE_N_DEPLOYMENTS=%u", new_deployments->len, - "OSTREE_SYNCFS_ROOT_MSEC=%" G_GUINT64_FORMAT, syncstats.root_syncfs_msec, - "OSTREE_SYNCFS_BOOT_MSEC=%" G_GUINT64_FORMAT, syncstats.boot_syncfs_msec, NULL); + ot_journal_send ("MESSAGE_ID=" SD_ID128_FORMAT_STR, + SD_ID128_FORMAT_VAL (OSTREE_DEPLOYMENT_COMPLETE_ID), "MESSAGE=%s", msg, + "OSTREE_BOOTLOADER=%s", + bootloader ? _ostree_bootloader_get_name (bootloader) : "none", + "OSTREE_BOOTLOADER_CONFIG=%s", bootloader_config, + "OSTREE_BOOTLOADER_ATOMIC=%s", bootloader_is_atomic ? "yes" : "no", + "OSTREE_DID_BOOTSWAP=%s", requires_new_bootversion ? "yes" : "no", + "OSTREE_N_DEPLOYMENTS=%u", new_deployments->len, NULL); _ostree_sysroot_emit_journal_msg (self, msg); } diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index 40498411..282df6d1 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -1076,10 +1076,30 @@ ensure_repo (OstreeSysroot *self, GError **error) return TRUE; if (!ensure_sysroot_fd (self, error)) return FALSE; + // TODO: Consider if we can use openat2 with RESOLVE_NO_XDEV self->repo = ostree_repo_open_at (self->sysroot_fd, "ostree/repo", NULL, error); if (!self->repo) return FALSE; + // Because the ostree model requires hardlinks, ensure up front + // here that /ostree is on the same filesystem as /ostree/repo. + // See `full_system_sync` which also requires this. + { + struct stat repo_stat, ostree_stat; + glnx_autofd int ostree_fd = -1; + if (!glnx_opendirat (self->sysroot_fd, "ostree", TRUE, &ostree_fd, error)) + return FALSE; + if (!glnx_fstat (ostree_fd, &ostree_stat, error)) + return FALSE; + if (!glnx_fstat (ostree_repo_get_dfd (self->repo), &repo_stat, error)) + return FALSE; + if (ostree_stat.st_dev != repo_stat.st_dev) + return glnx_throw (error, + "Unexpected state: ostree/ on device %" G_GUINT64_FORMAT + " but ostree/repo on device %" G_GUINT64_FORMAT, + (guint64)ostree_stat.st_dev, (guint64)repo_stat.st_dev); + } + /* Flag it as having been created via ostree_sysroot_get_repo(), and hold a * weak ref for the remote-add handling. */ diff --git a/tests/kolainst/destructive/staged-deploy.sh b/tests/kolainst/destructive/staged-deploy.sh index e2fe1b54..184da62c 100755 --- a/tests/kolainst/destructive/staged-deploy.sh +++ b/tests/kolainst/destructive/staged-deploy.sh @@ -54,6 +54,9 @@ EOF orig_mtime=$(stat -c '%.Y' /sysroot/ostree/deploy) systemctl show -p ActiveState ostree-finalize-staged.service | grep -q inactive + syncfs=$(journalctl --grep='Completed syncfs.*for system repo' | wc -l) + assert_streq "$syncfs" 2 + # Do the staged deployment ostree admin deploy --stage staged-deploy @@ -91,6 +94,10 @@ EOF # And there should not be a staged deployment test '!' -f /run/ostree/staged-deployment + # Also verify we did a syncfs run during finalization + syncfs=$(journalctl -b -1 -u ostree-finalize-staged --grep='Completed syncfs.*for system repo' | wc -l) + assert_streq "$syncfs" 2 + test '!' -f /run/ostree/staged-deployment ostree admin status > status.txt assert_not_file_has_content status.txt 'finalization locked' -- 2.30.2